Fix race in ref/deref of StringImpl for some cases #1372
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…main thread StringImpl's ref count in not thread safe.
The name StringImpl object can be shared with comositor thread if animaton source object has name StringImpl with ref count 1 (and so isSafeToSendToAnotherThread would return true). Which can lead to a race in StingImpl ref/deref between main and compositor threads.
|
Some of the code affected by this PR has changed upstream as part of the effort to use integer track ids in the long term. The referenced upstream change still keeps the string track ids, but there's ongoing work by @vivienne-w to fully migrate to integer ids. This means that this PR, if approved, would be downstream only. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes some random use-after-free reported by ASAN (when destroying SourceBufferPrivateGStreamer) and by Ref/Deref threading check applied on StringImpl object.
first commit addresses the race between main and streaming thread on track id string refcount. WebKitMediaSourceGStreamer references trackId in streaming thread when creating stream id, when notifying of low buffer level and for debugging purposes. The lifetime of trackId is guarantied to outlive streaming thread, so it should be enough to avoid copying the trackId or create an isolated copy when needed.
second commit addresses potential problem with the copy of Nicosia Animation. The Ref/Deref check report that same string is shared between main and compositor threads.
The Ref/Deref threading check implementation is available here:
emutavchi@724ef45. It is not included in this PR because it brings some overhead and may give false positive results (so reports should be reviewed carefully).